Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify behavior of interruptible map tasks #5845

Merged
merged 7 commits into from
Oct 23, 2024

Conversation

RaghavMangla
Copy link
Contributor

@RaghavMangla RaghavMangla commented Oct 14, 2024

Removed the retries option from map_tasks.md

Tracking issue

Closes: #4462

Why are the changes needed?

interruptible should work the same with maptasks as with regular python tasks — the retries field in the task annotation shouldn't be necessary.

What changes were proposed in this pull request?

Removed the retries field in task annotation from documentation, since its not necessary

How was this patch tested?

Setup process

Screenshots

Screenshots of html page rendered for map_tasks.md

Screenshot 2024-10-15 at 12 32 07 AM Screenshot 2024-10-15 at 12 32 14 AM Screenshot 2024-10-15 at 12 32 19 AM Screenshot 2024-10-15 at 12 32 37 AM Screenshot 2024-10-15 at 12 32 46 AM Screenshot 2024-10-15 at 12 32 51 AM Screenshot 2024-10-15 at 12 32 58 AM

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

github link: https://github.com/flyteorg/flyte/edit/master/docs/user_guide/advanced_composition/map_tasks.md
official link: https://docs.flyte.org/en/latest/user_guide/advanced_composition/map_tasks.html

Removed the retries option from map_tasks.md 

Signed-off-by: Raghav Mangla <[email protected]>
@RaghavMangla
Copy link
Contributor Author

Hi! @neverett @ppiegaze can u pls review the PR, I have attached the screenshots for HTML page rendered. Issue the PR is associated with, #4462

Copy link

codecov bot commented Oct 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.71%. Comparing base (56b6d6d) to head (620d691).
Report is 8 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5845      +/-   ##
==========================================
- Coverage   36.71%   36.71%   -0.01%     
==========================================
  Files        1304     1304              
  Lines      130081   130081              
==========================================
- Hits        47764    47758       -6     
- Misses      78147    78153       +6     
  Partials     4170     4170              
Flag Coverage Δ
unittests-datacatalog 51.58% <ø> (ø)
unittests-flyteadmin 54.39% <ø> (-0.03%) ⬇️
unittests-flytecopilot 11.73% <ø> (ø)
unittests-flytectl 62.40% <ø> (ø)
unittests-flyteidl 6.89% <ø> (ø)
unittests-flyteplugins 53.62% <ø> (ø)
unittests-flytepropeller 42.84% <ø> (ø)
unittests-flytestdlib 54.78% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@samhita-alla
Copy link
Contributor

@RaghavMangla i don't think this is necessary. instead, could you update the section on task retries to include all the relevant information Dan suggested?

@RaghavMangla
Copy link
Contributor Author

Hi, @samhita-alla I have reverted back my changes in map_tasks.md and made suggested changes in task retries docs,
github link: https://github.com/flyteorg/flyte/edit/master/docs/user_guide/flyte_fundamentals/optimizing_tasks.md

Screenshot 2024-10-15 at 5 42 38 PM Screenshot 2024-10-15 at 5 43 20 PM

can u pls review the changes and suggest if any changes are required

@RaghavMangla
Copy link
Contributor Author

Hi @samhita-alla can u pls review the PR and let me know if any changes are required, the pull request is linked to #4462

errors by randomly throwing a `RuntimeError` 5% of the time:
### Understanding Error Types

Flyte differentiates between two types of errors:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this section feels disconnected from the retries section. instead of simply copying and pasting, could you please try focusing on making it flow naturally as a single, cohesive section?

@RaghavMangla
Copy link
Contributor Author

RaghavMangla commented Oct 17, 2024 via email

@RaghavMangla
Copy link
Contributor Author

Hi, @samhita-alla I have updated with changes

image

Signed-off-by: RaghavMangla <[email protected]>
@RaghavMangla
Copy link
Contributor Author

Hi, @samhita-alla, can u pls provide a review abt the changes required, I have made the changes, and tried to keep the section continuous

@RaghavMangla
Copy link
Contributor Author

Hi! @neverett @samhita-alla @ppiegaze can u pls review the PR, (this is my fourth PR in hacktoberfest, so a 7 day verification would be required, could u pls share the changes if any today)

@RaghavMangla
Copy link
Contributor Author

Hi! @samhita-alla, @neverett @ppiegaze, can i get an update about the changes required

@RaghavMangla
Copy link
Contributor Author

Hi! @samhita-alla @neverett @ppiegaze, can i get an update about the changes required

@RaghavMangla
Copy link
Contributor Author

Hi, @neverett, I have made all the requested changes as asked, can you pls review the PR, if everything seems to be good, we can close the issue

system-level or catastrophic errors that may arise from issues that don't have
anything to do with user-defined code, like network issues and data center
outages.
Flyte's robust retry mechanism enhances the reliability of distributed computing environments by effectively managing task failures. This section delves into the configuration and application of retries, ensuring you can maximize task resilience and efficiency.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this block is needed. The old one seems to be more accurate

@davidmirror-ops
Copy link
Contributor

Also related to #3956

@RaghavMangla
Copy link
Contributor Author

updated

@RaghavMangla
Copy link
Contributor Author

Hi! @davidmirror-ops, what other changes are required

@davidmirror-ops davidmirror-ops changed the title Update map_tasks.md Clarify behavior of interruptible map tasks Oct 23, 2024
Copy link
Contributor

@davidmirror-ops davidmirror-ops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@davidmirror-ops davidmirror-ops merged commit 7e78e71 into flyteorg:master Oct 23, 2024
50 checks passed
@RaghavMangla
Copy link
Contributor Author

Hi team, can you please add me for contribution in docs

@davidmirror-ops
Copy link
Contributor

@RaghavMangla sure, please fill out this form https://tally.so/r/mJJ14r

@RaghavMangla
Copy link
Contributor Author

Form submitted, thank you for the opportunity!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docs] provide clarification in the docs regarding interruptible behavior when using map tasks
3 participants